feat(storageincentives): storage incentives phase 4 #4345

zelig · 2023-09-24T06:45:27Z

Co-authored-by: Viktor Levente Tóth <[email protected]>

istae · 2023-09-25T21:35:30Z

pkg/storageincentives/agent.go

 	sample, err := a.makeSample(ctx, storageRadius)
 	if err != nil {
 		return false, err
 	}

 	a.logger.Info("produced sample", "hash", sample.ReserveSampleHash, "radius", sample.StorageRadius, "round", round)

-	a.state.SetSampleData(round, sample, time.Since(now))
+	a.state.SetSampleData(round, sample)


why was the duration of the sampler removed?

The makeSample function saves the duration into a.metrics.SampleDuration and on rchash endpoint call, SampleWithProofs calculates the process time. This additional time handling seemed redundant.

the API returns the duration though

the current version also returns (storageincentives/agent.go:596)

https://github.com/ethersphere/bee/blob/master/pkg/api/redistribution.go#L68

the redistributionstate endpoints needs it

mrekucci · 2023-09-27T10:03:53Z

pkg/api/rchash.go

@@ -19,8 +19,9 @@ func (s *Service) rchash(w http.ResponseWriter, r *http.Request) {
 	logger := s.logger.WithName("get_rchash").Build()

 	paths := struct {
-		Depth   *uint8 `map:"depth" validate:"required"`
+		Depth   uint8  `map:"depth" validate:"min=0"`


The min=0 tag is not needed, the 0 is the minimum of an unsigned integer.

mrekucci · 2023-09-27T10:16:48Z

pkg/api/rchash.go

@@ -34,7 +35,14 @@ func (s *Service) rchash(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	resp, err := s.redistributionAgent.SampleWithProofs(r.Context(), anchor1, *paths.Depth)
+	anchor2, err := hex.DecodeString(paths.Anchor2)


I'd suggest to rather register a new pre-map hook (api.go:281):

"decHex": func(v string) (string, error) { buf, err := hex.DecodeString(v) return string(buf), err },

Then simply use map:"anchor2,decHex" above and caste []bytes(anchor1) when you want to use it. The same goes for the Anchor1.

mrekucci · 2023-09-27T10:18:53Z

pkg/bmt/bmt.go

@@ -40,6 +40,19 @@ type Hasher struct {
 	span   []byte      // The span of the data subsumed under the chunk
 }

+// facade


This comment should describe what this function does and should start with the: NewHasher... (by Golang convention).

mrekucci · 2023-09-27T10:40:27Z

pkg/bmt/proof.go

+	for i := p.size; i < p.maxSize; i += len(zerosection) {
+		_, err := p.Write(zerosection)
+		if err != nil {
+			return []byte{}, err


Return nil, err.

mrekucci · 2023-09-27T10:41:23Z

pkg/bmt/proof.go

+			return []byte{}, err
+		}
+	}
+	return p.Hasher.Hash(b)


Stay consistent, return p.Hash(b) will do.

it calls its ancestor's hash function that contains the logic for the hash

Stay consistent, return p.Hash(b) will do.

i dont think so

You're doing the same with p.Write instead of p.Hasher.Write above, which is also Hasher's method. So I'd recommend being consistent.

mrekucci · 2023-09-27T11:14:46Z

pkg/storageincentives/redistribution/inclusionproof.go

+	ChunkAddr  string `json:"chunkAddr"`
+}
+
+// Transforms arguments to ChunkInclusionProof object


Functin description should start with its name.

Still not fixed.

mrekucci · 2023-09-27T11:15:35Z

pkg/storageincentives/redistribution/inclusionproof.go

+}
+
+// Transforms arguments to ChunkInclusionProof object
+func NewChunkInclusionProof(proofp1, proofp2 bmt.Proof, proofp3 bmt.Proof, sampleItem storer.SampleItem) (ChunkInclusionProof, error) {


This proofp1, proofp2 bmt.Proof, proofp3 bmt.Proof can be rewriten as proofp1, proofp2, proofp3 bmt.Proof.

mrekucci · 2023-09-27T11:16:50Z

pkg/storageincentives/redistribution/inclusionproof.go

+}
+
+// Transforms proof object to its hexadecimal representation
+func newHexProofs(proof bmt.Proof) hexProof {


Function description should start with its name.

mrekucci · 2023-09-27T11:26:07Z

pkg/storageincentives/soc_mine_test.go

+
+// to generate uploads using the input
+// cat socs.txt | tail 19 | head 16 | perl -pne 's/([a-f0-9]+)\t([a-f0-9]+)\t([a-f0-9]+)\t([a-f0-9]+)/echo -n $4 | xxd -r -p | curl -X POST \"http:\/\/localhost:1633\/soc\/$1\/$2?sig=$3\" -H \"accept: application\/json, text\/plain, \/\" -H \"content-type: application\/octet-stream\" -H \"swarm-postage-batch-id: 14b26beca257e763609143c6b04c2c487f01a051798c535c2f542ce75a97c05f\" --data-binary \@-/'
+func TestSocMine(t *testing.T) {


The description is vague, so I'm not quite sure why this is necessary. If it exists solely to generate input for the soc endpoint test, then consider generating fixtures and writing a soc test using them.

yeah, this was meant to utilize the test's output and upload chunks trough the SOC API endpoint.
the test was running in a different environment where this test couldn't be called (no build tools installed etc.)
the comment was made by @zelig . shall we remove?

pkg/storer/sample.go

Co-authored-by: istae <[email protected]>

mrekucci

Apart from some comments, LGTM!

mrekucci · 2023-09-28T09:39:55Z

pkg/bmt/proof.go

+			return []byte{}, err
+		}
+	}
+	return p.Hasher.Hash(b)


You're doing the same with p.Write instead of p.Hasher.Write above, which is also Hasher's method. So I'd recommend being consistent.

mrekucci · 2023-09-28T09:43:08Z

pkg/storageincentives/agent.go

 }

+// Only called by rchash API


The method description should start with its name.

mrekucci · 2023-09-28T09:46:19Z

pkg/storageincentives/redistribution/inclusionproof.go

+	ChunkAddr  string `json:"chunkAddr"`
+}
+
+// Transforms arguments to ChunkInclusionProof object


Still not fixed.

This reverts commit 0890460.

Co-authored-by: Viktor Levente Tóth <[email protected]> Co-authored-by: nugaon <[email protected]> Co-authored-by: istae <[email protected]>

zelig added this to the si-phase4 milestone Sep 24, 2023

zelig self-assigned this Sep 24, 2023

bee-runner bot added the pull-request label Sep 24, 2023

zelig closed this Sep 24, 2023

zelig force-pushed the feat/phase4 branch from e4a2d09 to 9ce6f3c Compare September 24, 2023 20:30

zelig reopened this Sep 24, 2023

zelig force-pushed the feat/phase4 branch from e4a2d09 to a698d07 Compare September 24, 2023 20:39

zelig and others added 2 commits September 24, 2023 22:42

feat(bmt): bmt changes for storage incentives phase 4 (#4343)

930c52e

Co-authored-by: Viktor Levente Tóth <[email protected]>

feat(storageincentives): generate sample of SOCs for phase 4 test

0a9afe1

zelig force-pushed the feat/phase4 branch from a698d07 to 0a9afe1 Compare September 24, 2023 20:44

zelig changed the title ~~feat(storageincentives): main feature branch for storage incentives phase 4~~ feat(storageincentives): storage incentives phase 4 Sep 25, 2023

istae requested review from mrekucci and istae September 25, 2023 11:36

feat(ph4): agent and api (#4323)

f89ad31

istae reviewed Sep 26, 2023

View reviewed changes

nugaon added 2 commits September 26, 2023 16:23

fix: set sample duration into status

654aaa0

fix: pass duration in tests as well

1d0c465

nugaon requested a review from istae September 26, 2023 18:57

mrekucci reviewed Sep 27, 2023

View reviewed changes

nugaon and others added 4 commits September 27, 2023 17:30

chore: review

a4671f5

refactor: anchor deserialization

206c71d

revert: use p.Hasher.Hash instead of p.Hash

3d6c27e

fix: bump storage-incentives abi to v0.6.0-rc3 (#4348)

c53032d

Co-authored-by: istae <[email protected]>

istae approved these changes Sep 28, 2023

View reviewed changes

mrekucci approved these changes Sep 28, 2023

View reviewed changes

refactor: review

84efd85

nugaon merged commit 0890460 into master Sep 28, 2023

nugaon deleted the feat/phase4 branch September 28, 2023 11:35

istae added a commit that referenced this pull request Oct 3, 2023

revert: "feat(storageincentives): storage incentives phase 4 (#4345)"

c8c5957

This reverts commit 0890460.

istae added a commit that referenced this pull request Oct 3, 2023

feat(storageincentives): storage incentives phase 4 (#4345)

19e029c

Co-authored-by: Viktor Levente Tóth <[email protected]> Co-authored-by: nugaon <[email protected]> Co-authored-by: istae <[email protected]>

mrekucci pushed a commit that referenced this pull request Nov 13, 2023

feat(storageincentives): storage incentives phase 4 (#4345)

0de12b7

Co-authored-by: Viktor Levente Tóth <[email protected]> Co-authored-by: nugaon <[email protected]> Co-authored-by: istae <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storageincentives): storage incentives phase 4 #4345

feat(storageincentives): storage incentives phase 4 #4345

zelig commented Sep 24, 2023 •

edited by nugaon

Loading

istae Sep 25, 2023

nugaon Sep 26, 2023

istae Sep 26, 2023

nugaon Sep 26, 2023

istae Sep 26, 2023

istae Sep 26, 2023 •

edited

Loading

nugaon Sep 26, 2023

mrekucci Sep 27, 2023

mrekucci Sep 27, 2023

mrekucci Sep 27, 2023

mrekucci Sep 27, 2023

mrekucci Sep 27, 2023

nugaon Sep 27, 2023

zelig Sep 27, 2023

mrekucci Sep 28, 2023

mrekucci Sep 27, 2023

mrekucci Sep 28, 2023

mrekucci Sep 27, 2023

mrekucci Sep 27, 2023

mrekucci Sep 27, 2023

nugaon Sep 27, 2023

mrekucci left a comment

mrekucci Sep 28, 2023

mrekucci Sep 28, 2023

mrekucci Sep 28, 2023

feat(storageincentives): storage incentives phase 4 #4345

feat(storageincentives): storage incentives phase 4 #4345

Conversation

zelig commented Sep 24, 2023 • edited by nugaon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istae Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrekucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig commented Sep 24, 2023 •

edited by nugaon

Loading

istae Sep 26, 2023 •

edited

Loading